fix(Dropdown): keep selected value inside input for searchable single select#3405
fix(Dropdown): keep selected value inside input for searchable single select#3405rivka-ungar wants to merge 16 commits into
Conversation
… select The searchable single-select combobox previously forced the input value to null after selection and rendered the selected label as a visual overlay on top of an empty input. Screen readers read the input, not the overlay, so a selected combobox announced as "blank" — failing WCAG 2.1 SC 4.1.2 (Name, Role, Value, Level A). Stop overriding Downshift's default so the selected item's label lives inside the input and is exposed to assistive technologies. Supporting changes keep the component's existing behavior intact: - Seed initialInputValue with the selected label so a defaultValue/value is visible on mount (previously handled by the overlay). - Filter the list only on real user typing (InputChange); ignore the label Downshift writes into the input on selection/blur. - Reset the filter when the menu closes so reopening shows the full list, matching the prior behavior and the documented combobox pattern. The overlay in SingleSelectTrigger now naturally renders only for non-searchable single select (where inputValue stays null), so that path is unaffected. Multi-select uses a separate hook and is untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review by Qodo
1. DropdownInput props typed inline
|
PR Summary by QodoFix Dropdown a11y: keep selected label in input for searchable single select WalkthroughsDescription• Keep searchable single-select selection text in the input for WCAG 4.1.2 compliance. • Add multi-select a11y modes: text summary in input or keyboard-navigable chips. • Expand Storybook docs and update tests for new value-rendering behavior. Diagramgraph TD
A["Dropdown"] --> B("useDropdownCombobox") --> D["DropdownContext"]
A --> C("useDropdownMultiCombobox") --> D
D --> E["SingleSelectTrigger"]
D --> F["MultiSelectTrigger"] --> G["MultiSelectedValues"]
A --> H[["Storybook docs"]]
subgraph Legend
direction LR
_cmp["UI component"] ~~~ _hook("Hook") ~~~ _docs[["Docs/Stories"]]
end
High-Level AssessmentThe following are alternative approaches to this PR: 1. Keep overlay, sync an offscreen value for AT
2. Use aria-valuetext (or similar) instead of input value
3. Split into two PRs (single-select fix first, multi-select a11y modes second)
Recommendation: Current approach is the most robust for WCAG 4.1.2 because it uses the native/programmatic source of truth (the input value) rather than a parallel overlay. Keeping Downshift’s default inputValue on selection aligns with common combobox implementations and reduces AT-specific edge cases. For multi-select, providing explicit modes (textInput vs interactiveChips) is a pragmatic, opt-in path that preserves existing chip UI while enabling accessible value announcement and keyboard removal; alternatives that rely on hidden/aria-only value mirrors are typically more fragile. File ChangesEnhancement (7)
Bug fix (2)
Refactor (1)
Tests (1)
Documentation (4)
|
|
📦 Bundle Size Analysis ✅ No bundle size changes detected. Unchanged Components
📊 Summary:
|
Comprehensive story page covering all searchable single-select variants: overview, sizes, states, default/controlled value, icons & avatars, groups, tooltips, clearable/max-height, and custom filter / empty message. The default-value story demonstrates the selected value living inside the input. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Code review by qodo was updated up to the latest commit 6edf9ba |
Single-page accessibility reference covering the selected-value-in-input behavior, WCAG criteria, keyboard interaction, screen reader output, and the accessibility-relevant props (naming, state, feedback). Excludes layout props like size that do not affect accessibility. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Code review by qodo was updated up to the latest commit 3417b82 |
Convert the plain .md (which Storybook does not pick up) into an .mdx docs page for the Searchable single select group, with the live examples embedded. Trim to accessibility essentials: core selected-value-in-input behavior, WCAG 4.1.2, and the accessibility-relevant props. Dropped the generic keyboard and screen-reader sections and the broader WCAG list. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Code review by qodo was updated up to the latest commit 11b7f86 |
…single select The .selectedItem overlay was previously hidden for searchable mode only via the `!inputValue` guard, so it reappeared and coexisted with the input value whenever the input text was cleared while a selection remained. Gate the overlay on `!searchable` instead — for searchable single select the value lives inside the input, so the overlay must never render. Removes the now-dead `faded`/`hasSelected` classes and their styles. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Code review by qodo was updated up to the latest commit 127c22d |
…-off examples Add a clear "What changed" before/after section to the searchable single select accessibility page, and stories that demonstrate the text-only collapsed selected value: preselected start elements (icons/avatars), end elements (trailing icon, suffix/hint), and a custom valueRenderer that is not applied to the searchable selected display. Remove the Do/Don't section. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Code review by qodo was updated up to the latest commit d0ea70b |
MDX parses {curly braces} as JS expressions; "{selected label}" is not valid
JS and broke the Storybook/Chromatic preview build with an acorn parse error.
Replace it with plain text.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Looking for bugs?Check back in a few minutes. An AI review agent is analyzing this pull request. |
textInput: selected items are shown as a comma-separated summary in the input (WCAG 4.1.2 — exposes value to assistive technologies on focus). interactiveChips: chips stay visible alongside the input; keyboard nav via getSelectedItemProps — ArrowLeft/Right moves between chips, Backspace/Delete removes the focused chip, ArrowLeft from an empty input navigates to the last chip or the +N overflow badge. Chips overflow uses the existing useItemsOverflow hook for the +N count badge. Both modes keep the menu open on selection and support toggle (re-click to deselect). Default chip mode behavior is unchanged. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Storybook MDX page and stories for the textInput and interactiveChips props explaining how each addresses the WCAG 4.1.2 gap in default multi-select (empty input announced as blank by screen readers). Covers: - Side-by-side comparison of all three modes - textInput: comma-separated value in the input, exposed on mount, controlled usage, trade-offs - interactiveChips: keyboard navigation table, overflow (+N badge), trade-offs - Decision guide for choosing between modes - Common a11y props table (label, clearAriaLabel, error, etc.) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Code review by qodo was updated up to the latest commit 4fcb1dc |
|
Code review by qodo was updated up to the latest commit 1769c1d |
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| <div | ||
| className={styles.multiWrapper} | ||
| onKeyDown={e => { | ||
| if ( | ||
| e.key === "ArrowLeft" && | ||
| e.target instanceof HTMLInputElement && | ||
| !e.target.value && | ||
| overflowBadgeRef.current | ||
| ) { | ||
| overflowBadgeRef.current.focus(); | ||
| } | ||
| }} |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ipleSelection onStateChange UseMultipleSelectionStateChange has no selectedItem property — diff old vs new selectedItems array instead. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Code review by qodo was updated up to the latest commit f279286 |
|
A new prerelease version of this PR has been published! 🎉 |
| initialInputValue: inputValueProp ?? (textInput ? currentSelectedItems.map(i => i.label).join(", ") : ""), | ||
| id, |
There was a problem hiding this comment.
2. Stale textinput summary 🐞 Bug ≡ Correctness
In multi-select textInput mode, the input’s displayed summary is only seeded via initialInputValue and updated on internal Downshift events; when value is controlled and the parent changes value externally, there is no synchronization path to update Downshift’s inputValue, so the input can display a stale summary that no longer matches the actual selection.
Agent Prompt
### Issue description
For multi-select `textInput`, the input shows a comma-separated summary derived from selected items. But `useCombobox` is only initialized with that summary via `initialInputValue`, and there is no effect or controlled `inputValue` prop to keep it in sync when `value` (controlled selected items) changes outside of Downshift interactions.
### Issue Context
This causes UI divergence in common flows like form reset, server-driven updates, or parent state changes: `selectedItems` in context updates (because it uses `value`), but the input continues to show the old summary.
### Fix Focus Areas
- packages/core/src/components/Dropdown/hooks/useDropdownMultiCombobox.ts[28-118]
- packages/core/src/components/Dropdown/hooks/useDropdownMultiCombobox.ts[140-182]
### Suggested fix
Implement synchronization in `textInput` mode, for example:
- Maintain a dedicated `displayInputValue` state in the hook and pass it to `useCombobox` as `inputValue` (controlled), updating it on:
- user typing (InputChange)
- selection changes (when `currentSelectedItems` changes)
- close/blur (restore summary)
- Or, if available in your Downshift hook return, call `setInputValue(summary)` in a `useEffect` when `textInput` is true and `currentSelectedItems` changes, guarded so it doesn’t overwrite active user typing (e.g., only when menu is closed / input not focused).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
…ow example State the major issue prominently (selected chips live outside the input, so a multi-select with selections is announced as "blank" — failing WCAG 4.1.2), and add an InteractiveChipsOverflow example showing chips collapsing into a "+N" counter via minVisibleCount. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Code review by qodo was updated up to the latest commit 18df1ce |
| onIsOpenChange: ({ isOpen }) => { | ||
| // Reset the text filter when the menu closes so reopening always shows the full option list, | ||
| // even though the input keeps displaying the selected item's label. | ||
| if (!isOpen) { | ||
| filterOptions(""); | ||
| } | ||
| isOpen ? onMenuClose?.() : onMenuOpen?.(); | ||
| }, |
There was a problem hiding this comment.
1. Menu callbacks inverted 🐞 Bug ≡ Correctness
useDropdownCombobox (and the multi variant) fires onMenuClose when isOpen becomes true and onMenuOpen when it becomes false, reversing the intended lifecycle callbacks. Consumers relying on these events will run side-effects on the wrong transition.
Agent Prompt
## Issue description
The combobox hooks call `onMenuClose` on open and `onMenuOpen` on close (inverted), which breaks the semantic contract of these props.
## Issue Context
The select-based hooks already implement the correct mapping (`isOpen ? onMenuOpen : onMenuClose`), so the combobox hooks should match.
## Fix Focus Areas
- packages/core/src/components/Dropdown/hooks/useDropdownCombobox.ts[67-74]
- packages/core/src/components/Dropdown/hooks/useDropdownMultiCombobox.ts[89-93]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // Seed the input with the selected item's label so a defaultValue/value is visible (and exposed to | ||
| // assistive technologies) on mount, now that the selection lives inside the input rather than in an overlay. | ||
| initialInputValue: inputValueProp || selectedItem?.label || "", |
There was a problem hiding this comment.
2. Empty inputvalue overridden 🐞 Bug ≡ Correctness
useDropdownCombobox seeds initialInputValue with inputValueProp || selectedItem?.label, so a controlled empty string (inputValue="") is ignored and replaced by the selected label. This breaks controlled input behavior for consumers that intentionally set an empty input value on mount.
Agent Prompt
## Issue description
`initialInputValue` uses `||`, which treats an empty string as falsy and incorrectly falls back to `selectedItem?.label`.
## Issue Context
`inputValue` is a `string` prop (empty string is valid). The multi-combobox hook already uses `??` for this exact pattern.
## Fix Focus Areas
- packages/core/src/components/Dropdown/hooks/useDropdownCombobox.ts[60-63]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
…Trigger formatting Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Code review by qodo was updated up to the latest commit 30a6147 |
| const DropdownInput = ({ | ||
| inputSize, | ||
| fullWidth, | ||
| onKeyDown: externalKeyDown, | ||
| inputRef: externalInputRef | ||
| }: { | ||
| inputSize?: "small" | "medium" | "large"; | ||
| fullWidth?: boolean; | ||
| onKeyDown?: React.KeyboardEventHandler<HTMLInputElement>; | ||
| inputRef?: RefObject<HTMLInputElement>; | ||
| }) => { |
There was a problem hiding this comment.
1. dropdowninput props typed inline 📘 Rule violation ⚙ Maintainability
DropdownInput defines its props type inline in the component file and does not extend VibeComponentProps. This breaks the repo’s required prop typing convention and makes the component API harder to standardize and reuse.
Agent Prompt
## Issue description
`DropdownInput` has an inline props type in `DropdownInput.tsx` and it does not extend `VibeComponentProps`, which violates the component props typing convention.
## Issue Context
The PR adds new props (`onKeyDown`, `inputRef`) and types them inline, further expanding the component API without using the required `*.types.ts` pattern.
## Fix Focus Areas
- packages/core/src/components/Dropdown/components/Trigger/DropdownInput.tsx[9-19]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| disabled?: boolean; | ||
| readOnly?: boolean; | ||
| minVisibleCount?: number; | ||
| /** Extra props (tabIndex, onKeyDown, etc.) to spread on each visible chip container. */ | ||
| getChipContainerProps?: (item: Item, index: number) => Record<string, any>; | ||
| /** Ref forwarded to the +N overflow Chips element, for external keyboard focus management. */ | ||
| badgeRef?: React.Ref<HTMLDivElement>; | ||
| }; |
There was a problem hiding this comment.
2. multiselectedvaluesprops typed inline 📘 Rule violation ⚙ Maintainability
MultiSelectedValues keeps its props type (MultiSelectedValuesProps) defined inside the component file and it does not extend VibeComponentProps. This violates the required component props typing standard.
Agent Prompt
## Issue description
`MultiSelectedValuesProps` is defined inline in the component file and does not extend `VibeComponentProps`, contrary to the required typing convention.
## Issue Context
This PR expands the props surface area (e.g., `getChipContainerProps`, `badgeRef`) while keeping the props type local to the `.tsx` file.
## Fix Focus Areas
- packages/core/src/components/Dropdown/components/MultiSelectedValues/MultiSelectedValues.tsx[13-24]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| const DropdownInput = ({ | ||
| inputSize, | ||
| fullWidth, | ||
| onKeyDown: externalKeyDown, | ||
| inputRef: externalInputRef | ||
| }: { | ||
| inputSize?: "small" | "medium" | "large"; | ||
| fullWidth?: boolean; | ||
| onKeyDown?: React.KeyboardEventHandler<HTMLInputElement>; | ||
| inputRef?: RefObject<HTMLInputElement>; | ||
| }) => { |
There was a problem hiding this comment.
3. dropdowninput not forwardref 📘 Rule violation ⚙ Maintainability
DropdownInput is implemented as a plain function component and introduces an inputRef prop instead of using React.forwardRef. This violates the required forwardRef component pattern and can lead to inconsistent ref support across the design system.
Agent Prompt
## Issue description
`DropdownInput` does not use `React.forwardRef` and instead adds an `inputRef` prop, which conflicts with the compliance requirement that components use the forwardRef pattern.
## Issue Context
The component now accepts `inputRef?: RefObject<HTMLInputElement>` and manually chooses between internal vs external refs, but it still doesn’t expose a standard `ref` to consumers.
## Fix Focus Areas
- packages/core/src/components/Dropdown/components/Trigger/DropdownInput.tsx[9-19]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| <div | ||
| className={styles.multiWrapper} | ||
| onKeyDown={e => { | ||
| if ( | ||
| e.key === "ArrowLeft" && | ||
| e.target instanceof HTMLInputElement && | ||
| !e.target.value && | ||
| overflowBadgeRef.current | ||
| ) { | ||
| overflowBadgeRef.current.focus(); | ||
| } | ||
| }} |
There was a problem hiding this comment.
5. Backspace chip-nav missing 🐞 Bug ≡ Correctness
The interactiveChips prop documentation promises that pressing Backspace from the input moves focus to the last chip, but the implementation only handles ArrowLeft and never handles Backspace. As a result, keyboard-only users can’t perform the documented navigation in interactiveChips mode.
Agent Prompt
## Issue description
`interactiveChips`’ contract says Backspace (and ArrowLeft) from the input should move focus to the last chip. Current code only checks `ArrowLeft`, so Backspace does not move focus as documented.
## Issue Context
This is part of the new accessibility-focused feature set (`interactiveChips`). The docs/type comments and behavior should match to avoid misleading consumers and to ensure keyboard-only navigation works.
## Fix Focus Areas
- packages/core/src/components/Dropdown/components/Trigger/MultiSelectTrigger.tsx[41-70]
- packages/core/src/components/Dropdown/components/Trigger/DropdownInput.tsx[43-46]
- packages/core/src/components/Dropdown/Dropdown.types.ts[28-34]
## Suggested fix
Add a `Backspace` branch alongside `ArrowLeft` in the wrapper `onKeyDown`:
- Only trigger when the event target is the input and the input is empty.
- Prevent default/back-prop as needed so Backspace does not trigger any other removal behavior.
- Move focus directly to the last visible chip (or to the overflow badge only as an intermediate step if required), ensuring the documented behavior holds in both overflow and non-overflow cases.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
1 participant
Problem
The searchable single-select combobox forced the input value to
nullafter every selection and rendered the selected label as a visual overlay on top of an empty input.Screen readers read what's inside the input — not the overlay. So a combobox with an active selection announced as "Edit combo, collapsed, blank" (JAWS). The user had no way to know what they'd selected.
This fails WCAG 2.1 SC 4.1.2 — Name, Role, Value (Level A), which requires the current value of a form control to be programmatically determinable.
Fix
Stop overriding Downshift's default so the selected item's label lives inside the input, where assistive technologies can read it. The selected value is no longer a separate visual layer.
Supporting changes keep the component's existing behavior intact:
initialInputValueseeded with the selected label, so adefaultValue/controlledvalueis visible on mount (previously the overlay handled this).onInputValueChangefilters the list only on real user typing (InputChange), ignoring the label Downshift writes into the input on selection/blur.onIsOpenChangeresets the filter when the menu closes, so reopening shows the full option list — matching the component's prior behavior and the documented combobox pattern (same approach Chakra v3 / Ark UI recommend: reset the filter on open).The overlay in
SingleSelectTriggernow renders only for non-searchable single select (whereinputValuestaysnull), so that path is unaffected. Multi-select uses a separate hook and is untouched.Behavior parity
Aligned with Chakra v3 (Ark UI / Zag.js):
aria-selected+ activedescendant on selected option at openreset())Storybook
Adds a dedicated Components/Dropdown/Searchable single select page documenting all searchable single-select variants: overview, sizes, states, default/controlled value, icons & avatars, groups (sticky titles & dividers), tooltips, clearable / max-height, and custom filter / empty message. The default-value story demonstrates the selected value living inside the input.
Tests
searchable: false, since the indent-stripping overlay logic now applies only to non-searchable single select.Test plan
defaultValue/ controlledvalueshows the label in the input on mount🤖 Generated with Claude Code